Improve nullability annotations and generic variance #576
Conversation
|
Thanks @vlsi, very much appreciated! I tried using this branch in my project and get a Checker error using |
|
What is the error? |
|
It's a type mismatch. It's still |
|
@vlsi What's missing in your opinion to make it a mergeable, non-WIP PR? |
| @@ -454,7 +454,7 @@ public static CharacterArbitrary chars() { | |||
| * @return a new arbitrary instance | |||
| */ | |||
| @API(status = MAINTAINED, since = "1.1.1") | |||
| public static <T> Arbitrary<T> create(Supplier<T> supplier) { | |||
There was a problem hiding this comment.
I'm not sure that methods that previously returned <T> should now return <T extends @Nullable...>. It might be the right choice, but then it should be a different PR IMO. Or is it necessary to peacify the compiler?
There was a problem hiding this comment.
Do you mean you want forbid creating Arbitrary<String?> with this method?
Consider the use case: Arbitraries.create(() -> random.nextBoolean() ? "hi" : null).
Previous declaration (<T>) forbids such use cases, however, they are pretty valid.
There was a problem hiding this comment.
I'm with you that the new signature is correct, but it changes the previous return type. So I'd rather have it in a different PR. If it's too bothersome to pick those changes out, leave them in.
| @@ -217,7 +217,7 @@ public BuilderCombinator<B> in(BiFunction<B, @Nullable T, B> toFunction) { | |||
| * @param setter Use value provided by arbitrary to change a builder's property. | |||
| * @return new {@linkplain BuilderCombinator} instance with same embedded builder | |||
| */ | |||
| public BuilderCombinator<B> inSetter(BiConsumer<B, T> setter) { | |||
There was a problem hiding this comment.
Same argument here. If correct (have to think it through), then a different PR.
| @@ -79,6 +79,6 @@ default StreamableArbitrary<T, U> ofSize(int size) { | |||
| * @return new arbitrary instance | |||
| */ | |||
| @API(status = MAINTAINED, since = "1.7.3") | |||
| StreamableArbitrary<@Nullable T, U> uniqueElements(Function<@Nullable T, Object> by); | |||
| StreamableArbitrary<T, U> uniqueElements(Function<? super T, ?> by); | |||
There was a problem hiding this comment.
The change to the second type parameter should then also entail a change in @UniqueElements annotation and the classes that process it.
There was a problem hiding this comment.
Class<? extends Function<?, ?>> by() would allow specificly typed implementations. But it can also be done in a follow-up PR.
vlsi
changed the title
|
I think it should be good to go assuming the tests pass. Note: there might be many more generic variance improvements to come, and I touched only the signatures that were close to the nullability fixes. I'm fine answering questions and resolving the outstanding issues, however I will not spend my time on splitting the changes into different PRs. |
|
Many thanks @vlsi for the work and the focus on so many details! It also show how tedious it can become to implement null safety and variance constraints in a language that has not be designed for that. I'm hesitant if I'll aim for that in jqwik 2, too. |
This is a draft, however, it should more-or-less work.
Notable changes:
@NullMarkedpackage annotation does not automatically inherit to sub-packages, so@NullMarkedmust be duplicated manually (I've added manypackage-info.javafiles)<T> fun(value: T) {...means that it is valid to useString?forT. At the same time, Java defaults to non-null, so if nullable T should be allowed, then the declaration should be<T extends @Nullable Object>@NotNullis not needed. If IDEA suggest "overriding method does ..." and suggests adding@NotNull, then most likely@NullMarkedpackage-info.javais missingConsumer<T>->Consumer<? super T>;Function<ARG, RESULT>->Function<? super ARG, ? extends RESULT>and so on. Unfortuantely, with Java one has to duplicate those super/extends all over the placeList<? extends T>is effectively aReadOnlyList<T>. It is helpful in both generic variance (accepting more matching arguments) and it helps to understand the method signature as in "ok, the method acceptsList<? extends T>, so it does not modify the list"